Conversation
|
The line comment one is a bit tricky 'cause sometimes you don't want to break up a clean block of assignments but want a small comment next to one item. Otherwise, these all seem reasonable to me. |
README.md
Outdated
|
|
||
| - [12.13](#12.13) <a name="12.13"></a> Instance methods that don’t refer to `this` don’t need to be instance methods. If they relate to the class, make them static methods; otherwise, make them functions in scope. | ||
|
|
||
| > Why? This pattern is generally a sign that you are providing a bad public API for the class, and should either hide this method (if it’s an implementation detail) or expose it as a utility method. |
There was a problem hiding this comment.
maybe call it 'export' instead of 'expose'
lemonmade
force-pushed
the
update-deps
branch
from
a096644 to
f71e730
Compare
|
reping @Shopify/javascript. I think I am still leaning towards keeping the recommendation for line comments to appear above the line they are documenting, since I think that's usually the better choice and it's worth the consistency. Added a few more updates since I initially did this PR, so a few more things that might need discussion:
|
|
I don't think preferring module.exports or export matters because we recommend using ES6 module syntax I don't think we should have a preference of what flow comment style to use |
|
I think it's still worth having the rule for |
|
Keeping the |
|
lgtm |
4 participants
This PR updates all of our linting dependencies to their most recent versions and adds the new rules they introduced. Most notably, the following rules now apply:
Symbolfunction.Functionas a type (I chose not to restrictObjectandanysince I find them to be useful occasionally, not sure about it though).doneand returning a promise together in mocha tests.'expose Foo'.cc/ @Shopify/javascript @bouk thoughts?